Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix icmp type and code ranges #67

Merged
merged 4 commits into from
Sep 30, 2024
Merged

fix icmp type and code ranges #67

merged 4 commits into from
Sep 30, 2024

Conversation

haim-kermany
Copy link
Contributor

@haim-kermany haim-kermany commented Sep 27, 2024 •
edited by adisos
Loading

  • Example came from a real FW rule that used ICMPv6 type 135,136, which is not supported by current model within package netp. (found it was generated as connection-set object of ICMP 135,136, instead of skipping unsupported icmpv6).
  • No validation of type and code in connection.ICMPConnection(), and in connection.Set, resulted in strange union result which does not make sense. (description in issue icmp union strange result #65 )
  • Current fix in this PR is to use all 8-bit range values for ICMP type and code in connection.Set

@haim-kermany haim-kermany requested a review from adisos September 27, 2024 05:52
@adisos adisos changed the title test of icmp-type range fix icmp type and code ranges Sep 29, 2024
@adisos adisos linked an issue Sep 29, 2024 that may be closed by this pull request
icmp union strange result #65
Closed
@adisos
Copy link
Contributor

adisos commented Sep 29, 2024

@haim-kermany added a fix in this branch PR , using the 8-bit range for type and code

@adisos adisos requested a review from zivnevo September 29, 2024 09:05
zivnevo
zivnevo approved these changes Sep 30, 2024
View reviewed changes
Copy link
Member

@zivnevo zivnevo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to leave the old values as commented-out code?

@adisos
Copy link
Contributor

adisos commented Sep 30, 2024

Do we need to leave the old values as commented-out code?

no, removed

@adisos adisos merged commit 97f59b5 into main Sep 30, 2024
4 checks passed
@adisos adisos deleted the union-icmp branch September 30, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisos adisos adisos approved these changes

@zivnevo zivnevo zivnevo approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

icmp union strange result
3 participants
@haim-kermany @adisos @zivnevo